-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow Netherite provider to be implemented separately #1541
Conversation
@sebastianburckhardt , if this is WIP, should we consider making it a draft PR? :) |
Not sure what the difference is between draft and WIP? Since we just decided to restructure the projects I can't be quite sure what will change. Currently figuring this out. My guess is that in its eventual form, this PR will stay similar, as far as modifications to existing files are concerned, but it will not include the new durability provider. |
@sebastianburckhardt , a draft is a GitHub UI feature to acknowledge that a PR is work-in-progress. I've used it in the past to make a clean separation between PRs that need immediate reviews for immediate merge consideration and the PRs that do not. You can read about it here: Essentially if you CTRL+F "draft" in this page, you should be able to convert this into a draft PR. Just a suggestion! I can also do it for you if you'd like |
4e6b170
to
e2d768a
Compare
This is now ready to be reviewed. It only contains the changes needed for implementing Netherite as an external NuGet package. The changes include:
|
src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableClient.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me, I give it my thumbs-up but I do have suggestions for improvements, please see my comments.
I'll wait for a second pair of eyes to give this the approval 😄
src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableClient.cs
Outdated
Show resolved
Hide resolved
Ah it also seems like there's a merge conflict with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really happy with the latest changes. LGTM! Still waiting on a 2nd pair of eyes just in case
Re-started the CI build checks since it appears to have been running for over an hour and thus timed-out. Sometimes this happens, for some reason, with the CI. Hoping they pass now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks fairly good, just a few questions.
50eebc3
to
fe94721
Compare
…ds to public static class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Support for using the EventSourced backend, as in the work-in-progress PR Azure/durabletask#461.
Besides adding a durability provider, the changes include: